Skip to content

[SYSTEMDS-3730] Multi-threaded rev reorg operation#2290

Closed
j143 wants to merge 9 commits into
apache:mainfrom
j143:multi-thread-rev
Closed

[SYSTEMDS-3730] Multi-threaded rev reorg operation#2290
j143 wants to merge 9 commits into
apache:mainfrom
j143:multi-thread-rev

Conversation

@j143

@j143 j143 commented Jul 13, 2025

Copy link
Copy Markdown
Member

Basic multi-thread rev implementation.

the rev() Operation

Given a matrix $$( A )$$ of size $$( m \times n )$$, rev() produces a matrix $$( B )$$ of the same dimensions such that:

$$B_{i,j} = A_{m-1-i, j}$$

for all $$( 0 \leq i < m )$$ and $$( 0 \leq j < n )$$.

In words:

  • Each row of the output is the corresponding row from the bottom of the input matrix.
  • The last row of the input becomes the first row of the output, the second last becomes the second, ... the first row becomes the last.

Example

Suppose:

$$A = \begin{bmatrix} 1 & 2 & 3 \\ 4 & 5 & 6 \\ 7 & 8 & 9 \\ \end{bmatrix}$$

Then after rev(A), you get:

$$B = \begin{bmatrix} 7 & 8 & 9 \\ 4 & 5 & 6 \\ 1 & 2 & 3 \\ \end{bmatrix}$$


Multi-threaded Implementation

  • Split the matrix into chunks of rows.
  • Each thread copied its assigned chunk of output rows, each time taking from the appropriate (reversed) input row:
    • If a thread was responsible for output row $$( i )$$, it took input row $$( m-1-i )$$.
  • All threads worked in parallel.

Summary Table

Input Row $$( i )$$ Output Row $$( m-1-i )$$
0 last row
1 second last row
... ...
m-1 first row

test

for every cell $$( (i, j) )$$, output[i][j] == input[rows - 1 - i][j]

@j143 j143 marked this pull request as ready for review July 15, 2025 04:03
@j143

j143 commented Jul 15, 2025

Copy link
Copy Markdown
Member Author

Hi @mboehm7 , work on invoking the multi-threading part is pending. I verified the logic by directly hard coding number of threads earlier.

could you give some prelim comments on this.

@mboehm7 mboehm7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch @j143 - overall the changes are a very good start. Please address the minor comments and then I would merge it in.

Comment thread src/main/java/org/apache/sysds/runtime/matrix/data/LibMatrixReorg.java Outdated
Comment thread src/main/java/org/apache/sysds/runtime/matrix/data/LibMatrixReorg.java Outdated
}

public static MatrixBlock rev(MatrixBlock in, MatrixBlock out, int k) {
if (k <= 1 || in.isEmptyBlock(false)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a check for a minimum size for parallelization, otherwise fall back to single-threaded as we do it for example in aggregations (LibMatrixAgg).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added something like 3000_000 in the hop for now, for the tests to trigger the multi-thread.

// Set up thread pool
ExecutorService pool = CommonThreadPool.get(k);
try {
int blklen = (int) Math.ceil((double) numRows / k);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend to create smaller tasks (e.g., numRows/k/4) which yields better load balancing.

final int endRow = Math.min((i + 1) * blklen, numRows);

tasks.add(pool.submit(() -> {
if (!sparse) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create a static method for this kernel which is called from both the single-threaded implementation as well as the multi-threaded implementation

Comment thread src/main/java/org/apache/sysds/runtime/matrix/data/LibMatrixReorg.java Outdated
Comment thread src/main/java/org/apache/sysds/runtime/matrix/data/LibMatrixReorg.java Outdated
Comment thread src/main/java/org/apache/sysds/runtime/matrix/data/LibMatrixReorg.java Outdated
Comment thread src/main/java/org/apache/sysds/runtime/matrix/data/TestMultiThreadedRev.java Outdated

try
{
System.setProperty("sysds.parallel.threads", String.valueOf(numThreads));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove (I don't think we use this property internally)

@j143

j143 commented Jul 17, 2025

Copy link
Copy Markdown
Member Author

Hi @mboehm7 , thanks for review. At this point, the multi-threading is working. Can we checkpoint this PR till now and refine further in next PR? or need to update the handling.

Next:

  1. CP -> we decide threading, SP -> let the spark decide
  2. increase the test coverage
  3. use a common static block to use for both the single thread, multi-thread case
  4. decide on the blocksize or the threshold at which to trigger to threading

@mboehm7

mboehm7 commented Jul 17, 2025

Copy link
Copy Markdown
Contributor

Thanks @j143 , I would address then points 3 and 4 in the next days and subsequently merge it in.

@mboehm7

mboehm7 commented Jul 20, 2025

Copy link
Copy Markdown
Contributor

Thanks for the patch @j143. During the merge, I fixed the remaining stdout printing, moved the parallelization check from compilation to runtime, used the existing common kernels, fixed test errors (dense operations where running into null-pointer exceptions), and consolidated the tests.

@mboehm7 mboehm7 closed this in 559f770 Jul 20, 2025
@github-project-automation github-project-automation Bot moved this from In Progress to Done in SystemDS PR Queue Jul 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants